-
Notifications
You must be signed in to change notification settings - Fork 163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
plugin API rework, plugin status support #583
Conversation
pub trait PluginConditionSetter { | ||
fn add_error(self, report: &mut PluginReport) -> Self; | ||
fn add_warning(self, report: &mut PluginReport) -> Self; | ||
fn add_message(self, report: &mut PluginReport) -> Self; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As log levels we have "info, warn, err ,debug, trace". Do add_error
, add_warning
, and add_message
refer to those levels? If so, would add_message
better be named as add_info
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is related to plugin's error report level:
pub enum PluginReportLevel {
#[default]
Normal,
Warning,
Error,
}
I'll think about giving names which shows this relation more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, maybe Normal
could be renamed to Info
then? What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. This enum was PluginStatus
before, the Normal
remained from this. Now Info
is ok here.
@@ -31,6 +31,7 @@ pub struct FaceState { | |||
pub(super) id: usize, | |||
pub(super) zid: ZenohId, | |||
pub(super) whatami: WhatAmI, | |||
#[allow(dead_code)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is local
a dead_code
now? What kind of changes are introduced that make local
dead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a side-effect of hiding fileds in RuntimeState, which were public before (https://github.com/eclipse-zenoh/zenoh/pull/583/files#diff-d1c4d04e44c0a6c9db07b8efc60893ed69cdcdb7d8c970192a034f94280df339R51-R61) This caused large leak of private structures into public API. So the unused function FaceState::is_local()
was incorrectly public before.
Seems that this field and function can be removed, but better to ask @OlivierHecart first. So for now they just marked as dead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it looks good to me. I've left few minor comments that should be addressed before merging.
Done with all the comments. |
RunningPluginTrait
Corresponging PRs:
eclipse-zenoh/zenoh-backend-influxdb#54
eclipse-zenoh/zenoh-backend-rocksdb#40
eclipse-zenoh/zenoh-backend-filesystem#46
eclipse-zenoh/zenoh-plugin-webserver#32
eclipse-zenoh/zenoh-plugin-mqtt#32
eclipse-zenoh/zenoh-plugin-dds#168
eclipse-zenoh/zenoh-plugin-ros1#30
eclipse-zenoh/zenoh-plugin-ros2dds#51